feat: define Slurm config and plan contracts - #879
Conversation
Add strict authored configuration, profile, image, client, benchmark, dependency-lock, and resolved-plan records. Validate cross-record identities and digests with sanitized single-node and multi-node golden fixtures. Closes #873 Signed-off-by: Andre Manoel <amanoel@nvidia.com>
Signed-off-by: Andre Manoel <amanoel@nvidia.com>
Prevent secret material from entering persisted configuration and make nested contract collections immutable. Tighten client and benchmark semantic invariants with focused negative coverage. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Greptile SummaryThe PR introduces strict, immutable, versioned contracts for authored Slurm configuration and resolved execution plans.
|
| Filename | Overview |
|---|---|
| packages/data-designer-slurm/src/data_designer/slurm/config/run.py | Defines authored run contracts and now accepts canonical Hugging Face seed exports with null token fields while continuing to reject populated secret values. |
| packages/data-designer-slurm/src/data_designer/slurm/config/profiles.py | Defines profile catalogs, deterministic cluster-selection precedence, and digest-bound selected-profile validation. |
| packages/data-designer-slurm/src/data_designer/slurm/planning/models.py | Introduces immutable resolved-plan records for images, dependencies, placement, topology, sharding, and artifacts. |
| packages/data-designer-slurm/src/data_designer/slurm/planning/validation.py | Adds cross-record consistency checks for authored configuration, selected profiles, images, dependencies, placement, resources, and shards. |
| packages/data-designer-slurm/tests/contracts/test_config_records.py | Covers authored contract behavior, including the real canonical Hugging Face seed export associated with the previous review thread. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Authored["Authored Slurm configuration"] --> Validate["Strict contract validation"]
Catalog["Profile catalog"] --> Select["Deterministic profile selection"]
Select --> Plan["Immutable resolved plan"]
Validate --> Plan
Images["Image inspections and dependency locks"] --> CrossCheck["Cross-record validation"]
Plan --> CrossCheck
CrossCheck --> Client["Client execution records"]
CrossCheck --> Benchmark["Benchmark manifests and reports"]
Reviews (3): Last reviewed commit: "fix: close Slurm argument validation byp..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
nabinchha
left a comment
nabinchha
left a comment
There was a problem hiding this comment.
Thanks for putting this contract layer together, @andreatnvidia!
Summary
This PR establishes strict authored Slurm configuration, profile/image/client/benchmark records, and immutable resolved-plan records with deterministic serialization and broad cross-record validation. The implementation largely matches the stated intent, but a few boundary gaps still allow persisted secrets or records that are not actually bound to the authored inputs.
Findings
Critical — Let's fix these before merge
packages/data-designer-slurm/src/data_designer/slurm/config/run.py:325 — Secret-bearing vLLM flags can enter persisted config
- What:
validate_extra_args()only rejects flags in_OWNED_VLLM_FLAGS, so a supported vLLM argument such as--hf-token=plaintext-secretis accepted and emitted unchanged bymodel_dump()/serialize_json(). - Why: Authored configs and resolved plans are persisted records, so this leaks a bearer token despite the contract's guarantee that secret values are represented only through external references. vLLM explicitly supports
--hf-token, making this a reachable path rather than just an arbitrary malformed argument. - Suggestion: Apply
_is_secret_name()to the normalized option name inextra_argsand require those values to come fromenvironment: dict[EnvironmentName, EnvironmentBinding]viaSecretRef. Please cover both--hf-token=valueand split--hf-token,valueforms with negative tests.
packages/data-designer-slurm/src/data_designer/slurm/planning/validation.py:102 — Lock-file mode is not bound to the resolved dependency lock
- What: Cross-record validation compares
authored_requirementsonly whenrequirements is not None. When the authored config selectslock_file, neitherResolvedDependencyLocknorvalidate_resolved_plan()retains or verifies that source lock's identity/digest. I could authorlocks/user-lock.json, attach the existing golden dependency lock containingdata-designer-speech==0.2.0, update the plan's artifact digest, andvalidate_resolved_plan()still returned successfully. - Why: The validated plan may install dependencies unrelated to the lock file the user selected, defeating deterministic dependency resolution and the integrity boundary these records are meant to provide.
- Suggestion: Bind lock-file mode explicitly—for example, add the authored lock source as an
ArtifactReference(or its content digest) toResolvedDependencyLock, then compare it with the resolved source invalidate_resolved_plan(). Add a negative cross-record test where a valid but unrelated lock is substituted.
Warnings — Worth addressing
packages/data-designer-slurm/src/data_designer/slurm/config/profiles.py:217 — Profile provenance is not revalidated
- What:
validate_selected_profile()verifies the catalog digest and selected profile value, but not the claimedselection_source. A record naming thelabprofile withselection_source="default"validates even when the catalog's default isprimary; similarly, a hostname selection can claim a pattern that is not in the selected profile. - Why: Persisted selection provenance can disagree with the deterministic precedence rules while still passing the public validation function, so consumers cannot rely on the record to explain why that cluster was selected.
- Suggestion: Re-derive the source-specific invariants: require default selections to name
catalog.default_cluster, require hostname patterns to belong to the selected profile, and retain enough hostname evidence if the match itself must be replayed. Add forged-default and forged-pattern tests.
packages/data-designer-slurm/src/data_designer/slurm/config/run.py:265 — Requirement validation accepts malformed trailing content
- What: The non-URL branch uses
re.match()without validating the remainder of the string. For example,ClientDependencies(requirements=["not valid !!!"])is accepted as packagenot. - Why: Invalid authored dependency records survive the strict configuration boundary and fail later in the resolver, where the error is less direct and may occur after other planning work.
- Suggestion: Parse standard requirements with
packaging.requirements.Requirement(while retaining the stricter immutable-wheel rule for direct references), or otherwise full-match the entire supported grammar. Add malformed-suffix and incomplete-version tests.
packages/data-designer-slurm/src/data_designer/slurm/_contracts.py:214 — HTTP URL validation does not require a host
- What:
validate_url()acceptshttps:///missing-host, andRemoteMCPProviderConfigdoes not reject the resultingurlsplit(...).hostname is Nonevalue. - Why: A syntactically invalid MCP endpoint is accepted by the authored contract and only fails when the client attempts to connect.
- Suggestion: Validate the parsed scheme, hostname, and port (or use Pydantic's HTTP URL type) before applying the no-credentials/query/fragment checks. Add missing-host and invalid-port cases.
What Looks Good
- The frozen base models, recursive collection freezing, and byte-stable golden records make the process boundary explicit and testable.
- Placement, topology, ordered port claims, shard ranges, image inspection, and authored/plan digest relationships receive unusually thorough semantic validation.
- Test coverage is broad and behavior-oriented; the 153 contract tests and changed-file Ruff checks pass cleanly in the isolated review worktree.
Verdict
Needs changes — please close the persisted vLLM-secret path and bind authored lock-file inputs to their resolved lock before merge. The selection-provenance, requirement-parser, and URL-boundary gaps are also worth tightening while these v1 contracts are still being established.
This review was generated by an AI assistant.
Signed-off-by: Andre Manoel <amanoel@nvidia.com>
|
@nabinchha Thanks, these gaps were real. vLLM extra args now reject secret-shaped option names in both equals and split forms, and lock-file mode records an authored source plus a digest-bound ArtifactReference that validate_resolved_plan() binds back to the authored path. I also tightened profile provenance, switched requirement parsing to packaging.requirements.Requirement while retaining the immutable-wheel rules, and require MCP URLs to have a valid host and port. The dependency-lock goldens and plan digests now include the new source fields. |
nabinchha
left a comment
nabinchha
left a comment
There was a problem hiding this comment.
Thanks for the thoughtful follow-up, @andreatnvidia!
Summary
The new commit addresses the prior review threads around requirement parsing, remote MCP URLs, profile provenance, dependency-lock provenance, secret-shaped vLLM arguments, and canonical builder exports. The implementation now matches the stated contract much more closely, but two validation-boundary gaps remain.
Findings
Critical — Let's fix these before merge
packages/data-designer-slurm/src/data_designer/slurm/config/run.py:196,335 — Combined secret arguments bypass validation
- What: Both argument validators derive the option name by splitting only on
=. As a result,LocalStdioMCPProviderConfig(args=["--api-key sk-123"])andVllmServerConfig(extra_args=["--api-key sk-123"])are accepted and preserve the plaintext value inmodel_dump(). The same form also lets an owned vLLM option such as"--port 9000"bypass_OWNED_VLLM_FLAGS. - Why: These authored records are serialized, so the bypass reopens the secret-persistence path this validation is meant to close. It also allows compiler-owned arguments past the contract boundary, where they can fail late or conflict with generated invocation state.
- Suggestion: Extract the leading option name up to either
=or whitespace before applying the owned/secret checks (or reject whitespace-combined option/value entries and require separate argv tokens). Could we add regression coverage for the combined form in both the stdio MCP and vLLM validators?
Warnings — Worth addressing
packages/data-designer-slurm/src/data_designer/slurm/config/run.py:261 — Direct wheel URLs accept invalid ports
- What: The direct-wheel validator checks
parsed.hostnamebut never accessesparsed.port, soClientDependencies(requirements=["plugin @ https://example.test:invalid/plugin.whl#sha256=<64 hex chars>"])is accepted. - Why: This lets an invalid URL cross the strict authored-config boundary and fail only during dependency resolution. It is also inconsistent with the updated shared HTTP URL validator, which deliberately evaluates
parsed.portand normalizesValueError. - Suggestion: Validate
parsed.portinside atry/except ValueErrorbefore evaluating the wheel-specific constraints, or reuse the shared URL validation logic. A regression test with a nonnumeric port would lock this down.
What Looks Good
- The new
packaging.Requirementparsing closes the trailing-junk cases and gives normalized duplicate detection a solid foundation. - Binding authored lock-file paths to resolved artifacts, and revalidating default/hostname profile evidence, materially strengthens plan provenance.
- The canonical Hugging Face builder-export test is a good end-to-end regression for nullable secret fields, and the updated golden records make the new lock semantics easy to inspect.
Verdict
Needs changes — Please close the combined-argument secret/owned-flag bypass and reject invalid direct-wheel ports before merge.
This review was generated by an AI assistant.
Signed-off-by: Andre Manoel <amanoel@nvidia.com>
|
@nabinchha Good catches. The combined argument form was bypassing validation because normalization only split on |
nabinchha
left a comment
nabinchha
left a comment
There was a problem hiding this comment.
Thanks for the quick and thorough follow-up, @andreatnvidia!
Summary
The new commit closes the remaining argument-normalization and direct-wheel URL validation gaps. I rechecked the earlier findings as well, and the PR now matches the stated strict authored-config and resolved-plan contract.
Findings
No actionable findings.
What Looks Good
- The shared
_option_flag()helper applies one normalization rule to both stdio MCP and vLLM arguments, including combined values and leading whitespace. - Reusing
validate_url()gives direct wheel requirements the same host and port guarantees as the other HTTP boundaries while retaining the wheel-specific HTTPS and digest checks. - The new regression cases cover the exact bypasses from the previous review. All 171 contract tests pass, and the Slurm source and contract tests pass Ruff and formatting checks.
Verdict
Ship it — the previously requested changes are resolved and this is ready to merge.
This review was generated by an AI assistant.
2 participants
📋 Summary
Defines the strict, versioned authored configuration and immutable resolved-plan boundary for Data Designer Slurm. These contracts give configuration, planning, image, client-worker, and benchmark lanes deterministic shared records without importing Slurm tools or duplicating the runtime/state records in #878.
🔗 Related Issue
Closes #873
Related to #850. Companion to #878, which owns
data_designer.slurm.stateand the package's direct Pydantic dependency.🔄 Changes
✨ Added
BuilderConfigexports and the public shorthand builder shape.RunConfigpayloads instead of applying ambient defaults while loading a plan.slurm/state/, package metadata, wheel-install scripts, anduv.lockunchanged to avoid overlap with feat: define Slurm runtime and state record contracts #878.🔍 Attention Areas
🧪 Testing
.venv/bin/ruff check --fix . && .venv/bin/ruff format .make check-slurmmake test-slurm(109 passed)make test-slurm-wheel-installmake test- not run; validation was scoped to the optional Slurm package and isolated wheel boundary.✅ Checklist